- 
                Notifications
    You must be signed in to change notification settings 
- Fork 13.1k
Monomorphic flow nodes #57977
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Monomorphic flow nodes #57977
Conversation
| Looks like you're introducing a change to the public API surface area. If this includes breaking changes, please document them on our wiki's API Breaking Changes page. Also, please make sure @DanielRosenwasser and @RyanCavanaugh are aware of the changes, just as a heads up. | 
| @typescript-bot test it | 
| Hey @ahejlsberg, the results of running the DT tests are ready. Everything looks the same! | 
| @ahejlsberg Here are the results of running the user tests comparing  Everything looks good! | 
| @ahejlsberg Here they are:
 tscComparison Report - baseline..pr
 
System info unknown
 
Hosts
 
 
Scenarios
 
 
 Developer Information: | |||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| @ahejlsberg Here are the results of running the top 400 repos comparing  Everything looks good! | 
| @typescript-bot perf test this faster | 
| @ahejlsberg Here they are:
 tscComparison Report - baseline..pr
 
System info unknown
 
Hosts
 
 
Scenarios
 
 
 Developer Information: | |||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| @typescript-bot perf test this faster | 
| @typescript-bot perf test this faster | 
| @ahejlsberg Here they are:
 tscComparison Report - baseline..pr
 
System info unknown
 
Hosts
 
 
Scenarios
 
 
 Developer Information: | |||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| @ahejlsberg Here they are:
 tscComparison Report - baseline..pr
 
System info unknown
 
Hosts
 
 
Scenarios
 
 
 Developer Information: | |||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| @typescript-bot perf test this faster | 
| @typescript-bot perf test this faster | 
| Hey @ahejlsberg, the results of running the DT tests are ready. Everything looks the same! | 
| @ahejlsberg Here are the results of running the user tests comparing  Everything looks good! | 
| @ahejlsberg Here they are:
 tscComparison Report - baseline..pr
 
System info unknown
 
Hosts
 
 
Scenarios
 
 
 Developer Information: | |||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| @ahejlsberg Here they are:
 tscComparison Report - baseline..pr
 
System info unknown
 
Hosts
 
 
Scenarios
 
 
 Developer Information: | |||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| Generally positive impact on performance with check time savings of up to 1.5%. Note that this is an API breaking change due to the more uniform layout of flow nodes. | 
| @ahejlsberg Here are the results of running the top 400 repos comparing  Everything looks good! | 
| 
 To be honest, I'm not sure why any of  But, the API break will probably break the playground's flow node viewer and ts-ast-viewer.com's, but we can probably update both. Realistically the former should have been calling into  | 
| id?: number; // Node id used by flow type cache in checker | ||
| id: number; // Node id used by flow type cache in checker | ||
| node: unknown; // Node or other data | ||
| antecedent: FlowNode | FlowNode[] | undefined; | 
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm kind of surprised that this turned out okay; I would have thought these two types would have different shapes, but I guess they're both pointers...
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
They're both heap objects so should get the same hidden class.
| I opened #58036 which would remove the flow types from the public API, and make this change "non breaking" in a way 😄 | 
| @jakebailey Maybe we just fold the  | 
| I figured it may be better not to, just to make it clear that we intentionally removed them, but I'm not against it if we're good with removing them. If #58036 is merged first, then your PR doesn't have any breaks (no baseline changes at all). | 
| 
 Fine by me. | 
# Conflicts: # src/compiler/checker.ts
| Done  | 
# Conflicts: # src/compiler/types.ts # tests/baselines/reference/api/typescript.d.ts
# Conflicts: # src/compiler/binder.ts
Experiment to observe the performance effects of monomorphic flow nodes.